Use Meyers singletons by leekillough · Pull Request #1222 · sstsimulator/sst-core

leekillough · 2025-02-25T01:23:36Z

Use Meyers Singletons (static variables local to functions which are initialized on their first call and return a reference to it; avoids static initialization order fiasco) instead of pointers allocated on demand.
Use std::shared_ptr for LoadedLibraries loaded libraries so that libraries and aliases can share the same loader pointers without memory leaks. I was prompted to do this by valgrind:

==3714114== 80 bytes in 1 blocks are definitely lost in loss record 1,155 of 1,554
==3714114==    at 0x4859047: operator new(unsigned long) (vg_replace_malloc.c:472)
==3714114==    by 0x249295: addLoader (elementinfo.h:266)
==3714114==    by 0x249295: addInfo (elementinfo.h:184)
==3714114==    by 0x249295: addInfo (statbase.h:426)
==3714114==    by 0x249295: bool SST::Statistics::Statistic<void>::addDerivedInfo<SST::Statistics::NullStatistic<void> >(std::__cxx\
11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<cha\
r>, std::allocator<char> > const&) (statbase.h:426)
==3714114==    by 0x211E3F: add<SST::Statistics::NullStatistic<void> > (elementinfo.h:281)
==3714114==    by 0x211E3F: __static_initialization_and_destruction_0 (elementinfo.h:286)
==3714114==    by 0x211E3F: _GLOBAL__sub_I_main (main.cc:1281)
==3714114==    by 0x5319BBD: call_init (libc-start.c:145)
==3714114==    by 0x5319BBD: __libc_start_main@@GLIBC_2.34 (libc-start.c:347)
==3714114==    by 0x22E0A4: (below main) (in /home/lkillough/sstcore-14.1.0/libexec/sstsim.x)

==3714114== 80 bytes in 1 blocks are definitely lost in loss record 1,156 of 1,554
==3714114==    at 0x4859047: operator new(unsigned long) (vg_replace_malloc.c:472)
==3714114==    by 0x335C47: addLoader (elementinfo.h:266)
==3714114==    by 0x335C47: addInfo (elementinfo.h:184)
==3714114==    by 0x335C47: SST::RealTimeAction::addInfo(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<ch\
ar> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, SST::ELI::BuilderInfoImpl<SST\
::ELI::ProvidesDefaultInfo, void>*) (realtime.cc:90)
==3714114==    by 0x247986: addDerivedInfo<SST::ExitCleanRealTimeAction> (realtimeAction.h:32)
==3714114==    by 0x247986: bool SST::ELI::ElementsInfo<SST::RealTimeAction>::add<SST::ExitCleanRealTimeAction>() (elementinfo.h:281)
==3714114==    by 0x212037: __static_initialization_and_destruction_0 (elementinfo.h:286)
==3714114==    by 0x212037: _GLOBAL__sub_I_main (main.cc:1281)
==3714114==    by 0x5319BBD: call_init (libc-start.c:145)
==3714114==    by 0x5319BBD: __libc_start_main@@GLIBC_2.34 (libc-start.c:347)
==3714114==    by 0x22E0A4: (below main) (in /home/lkillough/sstcore-14.1.0/libexec/sstsim.x)
==3714114==

I can see other places where std::shared_ptr and std::unique_ptr should be used, but since the memory system is being overhauled, I won't touch the other places.

Use C++17 inline with static const class members so that they can be defined in-class and won't have multiple definition errors if defined in a header.

initialized on their first call and return a reference to it; avoids static initialization order fiasco) instead of pointers allocated on demand. Use std::shared_ptr for LoadedLibraries loaded libraries so that libraries and aliases can share the same loader pointers without memory leaks. Use C++17 "inline" with static const members so that they can be initialized in-class and won't have multiple definition errors if defined in a header.

github-actions · 2025-02-25T01:23:58Z

CLANG-FORMAT TEST - PASSED

github-actions · 2025-02-25T01:24:08Z

CMAKE-FORMAT TEST - PASSED

leekillough · 2025-02-25T01:24:27Z

src/sst/core/elemLoader.cc

                for ( auto& elempair : libpair.second ) {
                    // loop all the loaders in the element
-                    for ( auto* loader : elempair.second ) {
+                    for ( auto& loader : elempair.second ) {


This allows loader to be either a regular pointer or a smart pointer. The previous code only allowed regular pointers.

leekillough · 2025-02-25T01:27:32Z

src/sst/core/eli/elementbuilder.h

-            return iter->second;
-        }
-    }
+    BaseBuilder* getBuilder(const std::string& name) { return factories_[name]; }


Since I do not see anywhere factories_ is used in such a way that adding nullptr entries would be a problem, this function can simply reduced to this subscript operator, which will create and return a nullptr entry if the name doesn't exist.

leekillough · 2025-02-25T01:31:50Z

src/sst/core/eli/elementbuilder.h

+        static Map libraries; // Database
+        auto&      lib = libraries[name];
+        if ( !lib ) lib = new Library(name);
+        return lib;


This is the Meyers singleton initializing the map on the first call to getLibrary().

It also uses a reference to the map entry and checks if it's nullptr, and if it is (such as if it did not exist before), sets it to allocated memory.

leekillough · 2025-02-25T01:35:28Z

src/sst/core/eli/elementbuilder.h

    static bool isLoaded() { return loaded; }

-    static const bool loaded;
+    static inline const bool loaded = Base::Ctor::template add<T>();


inline can be used with static class data members in C++17, and they can be initialized in-class. This prevents multiple definitions which could occur in the old code if the variable was ODR-used (had its address taken), since it was defined in the header file.

static constexpr member variables are inline by default, but the initializer here is not constexpr (it possibly could be made so, if the operator() and its return value were constexpr).

leekillough · 2025-02-25T01:37:52Z

src/sst/core/eli/elementbuilder.h

-        if ( !cached_ ) { cached_ = new T(std::forward<Args>(ctorArgs)...); }
-        return cached_;
+        static T cached(std::forward<Args>(ctorArgs)...);
+        return &cached;


Another Meyers Singleton, in lieu of calling new if a pointer is nullptr (I did not see this CachedAllocator used anywhere, and I doubt any code calls delete on its returned address, so returning the address of a static is okay).

If it needs to be a pointer which can be deleted, it simply needs to be changed to:

static T* cached = new T(std::forward<Args>(ctorArgs)...); return cached;

leekillough · 2025-02-25T01:38:30Z

src/sst/core/eli/elementinfo.h

+    {
+        static std::map<std::string, std::map<std::string, T*>> infos_;
+        return infos_;
+    }


Another Meyers singleton.

leekillough · 2025-02-25T01:39:38Z

src/sst/core/eli/elementinfo.h

+    {
+        static Map libs;
+        return libs;
+    }


leekillough · 2025-02-25T01:40:16Z

src/sst/core/eli/elibase.cc

-    if ( !alias.empty() ) (*loaders_)[lib][alias].push_back(loader);
-    return true;
-}
+    std::shared_ptr<LibraryLoader> shared_loader(loader);


We create a std::shared_ptr which owns the loader pointer passed in.

leekillough · 2025-02-25T01:43:54Z

src/sst/core/eli/elibase.cc

-    }
+    auto& library = getLoaders()[lib];
+    if ( !alias.empty() && alias != name ) library[alias].push_back(shared_loader);
+    library[name].push_back(std::move(shared_loader));


On the last use of shared_loader we use std::move() so that it doesn't have the overhead of incrementing the reference count during the push_back() and then decrementing the reference count when shared_loader is destroyed. We move shared_loader to the new entry without changing reference counts, and its destruction is a no-op.

leekillough · 2025-02-25T01:49:28Z

src/sst/core/eli/elibase.h

 {
 public:
-    using InfoMap    = std::map<std::string, std::list<LibraryLoader*>>;
+    using InfoMap    = std::map<std::string, std::deque<std::shared_ptr<LibraryLoader>>>;


std::deque is faster than std::list and std::vector for adding an unknown number of items repeatedly to the end of a list which grows. std::list requires more dynamic memory allocations and is cache-unfriendly, and std::vector requires moving the entire vector's data to a new location if it has to increase its capacity. std::deque is also randomly accessible like std::vector, but it's not contiguous. I would look at all places std::vector and std::list are used and see if std::deque might be better.

leekillough · 2025-02-25T01:50:17Z

src/sst/core/eli/elibase.h

+    {
+        static LibraryMap loaders;
+        return loaders;
+    }


Meyers singleton.

sst-autotester · 2025-02-25T01:51:42Z

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

berquist · 2025-03-04T16:29:29Z

@leekillough poke for rebase

sst-autotester · 2025-03-04T19:53:54Z

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

github-actions · 2025-03-05T19:11:53Z

CMAKE-FORMAT TEST - PASSED

sst-autotester · 2025-03-06T01:49:55Z

Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
THE LAST COMMIT TO THIS PULL REQUEST HAS BEEN REVIEWED, BUT NOT ACCEPTED OR REQUIRES CHANGES!

sst-autotester · 2025-03-06T01:50:01Z

All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur...

github-actions · 2025-03-06T03:11:16Z

CLANG-FORMAT TEST - PASSED

github-actions · 2025-03-06T03:11:28Z

CMAKE-FORMAT TEST - PASSED

sst-autotester · 2025-03-06T03:19:54Z

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

github-actions · 2025-03-06T03:21:05Z

CLANG-FORMAT TEST - PASSED

github-actions · 2025-03-06T03:21:16Z

CMAKE-FORMAT TEST - PASSED

sst-autotester · 2025-03-10T15:19:49Z

Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed.

sst-autotester · 2025-03-10T15:25:18Z

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

Build Num: 1857
Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

Build Num: 1813
Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

Build Num: 1812
Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore

Build Num: 810
Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

Build Num: 663
Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core

Build Num: 617
Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

Build Num: 380
Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore

Build Num: 235
Status: STARTED

Using Repos:

Repo: CORE (leekillough/sst-core)

Repo: SQE (sstsimulator/sst-sqe)

Repo: ELEMENTS (sstsimulator/sst-elements)

Repo: MACRO (sstsimulator/sst-macro)

Pull Request Author: leekillough

sst-autotester · 2025-03-10T16:47:20Z

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

Build Num: 1857
Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

Build Num: 1813
Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

Build Num: 1812
Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore

Build Num: 810
Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

Build Num: 663
Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core

Build Num: 617
Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

Build Num: 380
Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore

Build Num: 235
Status: PASSED

sst-autotester · 2025-03-10T16:47:38Z

Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ feldergast ]!

sst-autotester · 2025-03-10T16:47:45Z

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

* Use Meyers singletons (static variables local to functions which are initialized on their first call and return a reference to it; avoids static initialization order fiasco) instead of pointers allocated on demand. Use std::shared_ptr for LoadedLibraries loaded libraries so that libraries and aliases can share the same loader pointers without memory leaks. Use C++17 "inline" with static const members so that they can be initialized in-class and won't have multiple definition errors if defined in a header.

github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CLANG-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Feb 25, 2025

github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CMAKE-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Feb 25, 2025

leekillough commented Feb 25, 2025

View reviewed changes

src/sst/core/eli/elementinfo.h

{

static Map libs;

return libs;

}

Copy link

Contributor Author

leekillough Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again.

leekillough commented Feb 25, 2025

View reviewed changes

src/sst/core/eli/elibase.h

{

static LibraryMap loaders;

return loaders;

}

Copy link

Contributor Author

leekillough Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meyers singleton.

leekillough added 2 commits March 4, 2025 13:48

Merge remote-tracking branch 'sstcore/devel' into Use_Meyers_Singletons

d2e165f

fix merge

6cc6612

Merge remote-tracking branch 'sstcore/devel' into Use_Meyers_Singletons

b818ea3

github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Mar 5, 2025

github-actions bot added the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Mar 5, 2025

don't use auto for functions

11bd11c

github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Mar 6, 2025

make CachedAllocator() use allocated memory

a09dc37

github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Mar 6, 2025

berquist added the AT: PRE-TEST INSPECTED label Mar 10, 2025

sst-autotester removed the AT: PRE-TEST INSPECTED label Mar 10, 2025

feldergast approved these changes Mar 10, 2025

View reviewed changes

feldergast merged commit 86ef30f into sstsimulator:devel Mar 10, 2025
7 checks passed

leekillough deleted the Use_Meyers_Singletons branch March 31, 2025 04:05

berquist added this to the SST V15.0.0 milestone Apr 14, 2025

leekillough mentioned this pull request Apr 15, 2025

Change raw pointers to unique_ptr for static variables in new ELI #394

Closed

Conversation

leekillough commented Feb 25, 2025

Uh oh!

github-actions bot commented Feb 25, 2025

Uh oh!

github-actions bot commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sst-autotester commented Feb 25, 2025

Uh oh!

berquist commented Mar 4, 2025

Uh oh!

sst-autotester commented Mar 4, 2025

Uh oh!

github-actions bot commented Mar 5, 2025

Uh oh!

sst-autotester commented Mar 6, 2025

Uh oh!

sst-autotester commented Mar 6, 2025

Uh oh!

github-actions bot commented Mar 6, 2025

Uh oh!

github-actions bot commented Mar 6, 2025

Uh oh!

sst-autotester commented Mar 6, 2025

Uh oh!

github-actions bot commented Mar 6, 2025

Uh oh!

github-actions bot commented Mar 6, 2025

Uh oh!

sst-autotester commented Mar 10, 2025

Uh oh!

sst-autotester commented Mar 10, 2025

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore

Using Repos:

Uh oh!

sst-autotester commented Mar 10, 2025

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

Build Information